-
Notifications
You must be signed in to change notification settings - Fork 18
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Updating Branch Predictors to replace hash-map with Fetch Target Queue #401
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have some reservations:
My fear here is that we are starting to stray from the very generic functionality of a BranchPredictor. We add the none generic addToFTQ function that may not be required by all future predictors. Update and other functions now need to be called in a very specific order which mandates where they can be called in the pipeline. This may not play well with predictors that don't have structures like an FTQ and also means it can't get early prediction feedback i.e. update called at commit and not at the end of exec unit which could be a huge delay for feedback e.g. up to 630 entries in an M1 ROB (commit width of, say, 8 means best case delay of 79 cycles!!!) . However, I understand that to have this FTQ functionality all of this is required so I'm not sure what a good solution is to keep predictors generic but also allow more tightly integrated behaviour.
We also now need to be REALLY REALLY careful about the order in which we flush the predictor now. Even changing the order of units being flushed in the core.flush function will break the BPs logically (potentially), but will be really hard to see/find. Potentially we need to add some debug behaviour to ensure that we ALWAYS pop the correct entry e.g. only in debug mode, each entry in the FTQ also stores the associated address and there are assertions to ensure that flush addresses match up. I just think this behaviour in the current implementation is too easy to break and too subtle to realise that you have broken it.
Regardless I think we should move all predictors into their own directory now that we have a good selection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally very clean. Most comments are minor points or questions
Unit tests need adding for PipelineBuffers
for the new branchg flushing functionality added. Its also worth manking sure all new added functionality has an associated unit test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work - couldn't spot anything bar a tiny spelling error.
Commendable work on commenting the code - makes it really clear as a dev to see what's going on with the BP.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am looking forward to this being merged
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work and nearly there. A couple minor points, but these shouldn't take too long.
Please note that my comment about mask creation using the wrong types/overflowing impacts a few areas in the code, but was difficult to tell when I'm not certain on the types each variable uses. This shouldn't take too long to check, but likely faster for you than me if you're familiar with the types you set things to.
Will be nice to get this one merged :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All looks great to me!!!
The merge-base changed after approval.
The merge-base changed after approval.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good!
The merge-base changed after approval.
The merge-base changed after approval.
* branch instruction, flushes them. | ||
*/ | ||
void flushBranchesInBufferFromSelf( | ||
pipeline::PipelineBuffer<std::shared_ptr<Instruction>>* buffer) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless I'm mistaken, this is a pointer not a reference. For a reference, you'd need to swap *
with &
and pass in the buffer param in as normal.
The merge-base changed after approval.
The merge-base changed after approval.
This update to the branch predictors (both Perceptron and Generic) replaces the hashmap that was previously being used to keep previous-state information between predict and update with a queue (called a Fetch Target Queue or FTQ) of previous-state information for the in-flight branches. The FTQ allows speculative updating of the global history (which is also included in this PR).
In terms of performance, it seems to be quite lateral. a full breakdown of stats across our benchmark suite is provided below. To summarise the results, most benchmarks have an improved misprediction rate with the update, but the changes in execution time are hit and miss with many becoming a bit slower (probably due to an increased number of interactions with the branch predictor through the pipeline).
I think this is still a worthwhile change to make to SimEng even though there isn't a clear-cut performance upgrade because it makes the flow of branches through the branch predictor more regular and so should facilitate more complicated branch predictors in the future (e.g., TAGE).